Use --! Included blocks on commit so uncommit can undo includes#211
Use --! Included blocks on commit so uncommit can undo includes#211benjie merged 14 commits intographile:mainfrom
--! Included blocks on commit so uncommit can undo includes#211Conversation
|
Thanks for taking a stab at this, @astephensen 🙂 |
I think you must have it misconfigured (older prettier version?) - use |
benjie
left a comment
There was a problem hiding this comment.
This looks really good to me. The main things I'd like to see to get this merged are:
- Must refuse to run/commit/process user-authored migrations that contain
--! Includedor--! EndIncludedcomments. Save people from copy/paste issues. - Extend the tests to cover multi-file migrations - want to ensure that the
--! Included/--! EndIncludedinteract fine with file splits. (Not anticipating any issues, but always good to have tests.) Both for commit and uncommit.
|
Thanks for the review @benjie, I will address those comments! |
|
This is also necessary for a commit rollback - if there is a consistency error and |
|
@astephensen Are you still interested in pushing this through? I'd take over if not |
Co-authored-by: Benjie <benjie@jemjie.com>
benjie
left a comment
There was a problem hiding this comment.
I think this is essentially ready to merge. I've not manually tested it yet though.
--! Included blocks on commit so uncommit can undo includes
|
Released in |
Description
This PR will wrap included files with comments, as first described in #208.
This also facilitates removing the included content when performing an uncommit.
Have opened as a draft as I'm open to any and all feedback!
Pre-Commit
current.sqlfixtures/functions/hello.sqlCommitted
migrations/000001-world.sqlUncommitted
current.sqlImplementation
The method I implemented is similar to what @jcgsville described in the original issue, with a few little changes.
--! Include:being treated as an actual header if the first statement was an include. Figured dropping the colon helps distinguish this as a file include, but I'm open to suggestions on how to fix this!--! EndIncluded— This allows a simple regex back reference to be used to delete the included file (if the filename wasn't added, nested includes could be trickier to remove).The main downside of this approach is the original formatting of the
--!includeis not preseved, i.e. if the include is written as--! include ...the migration will not restore the additional whitespace when uncommitting.Notes
Performance impact
unknown - likely minimal.
Security impact
unknown - likely none.
Checklist
yarn lint:fixpasses.yarn testpasses.I have added this feature to 'Pending' in the- Does not existRELEASE_NOTES.mdfile (if one exists).If this is a breaking change I've explained why.- Not a breaking change